-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: automatic Chrome dependency installation and Docker browser support for Codespaces #7995
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… support for Codespaces - Implement automatic detection and installation of Chrome dependencies - Add optional Docker-based browser isolation with configurable settings - Create comprehensive devcontainer configuration for GitHub Codespaces - Add environment detection for Codespaces, containers, and Linux systems - Implement fallback mechanisms: system Chrome -> Docker -> downloaded Chrome - Add new VS Code settings for Docker browser configuration - Include post-create and post-start scripts for Codespaces setup - Add comprehensive tests for browser environment functionality Fixes #7990
| } | ||
|
|
||
| // Launch the browser | ||
| const stats = await this.ensureChromiumExists() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid duplicate calls to ensureChromiumExists(). The method is called in the else branch to set the executablePath and then called again for launching the browser. Consider reusing the result to prevent unnecessary downloads or repeated work.
| args.push("--no-sandbox", "--disable-setuid-sandbox") | ||
| } | ||
|
|
||
| const stats = await this.ensureChromiumExists() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to BrowserSession, ensureChromiumExists() is called again for launching even after determining executablePath. Consider refactoring so that the puppeteer launcher is obtained without re-triggering a potential download when system Chrome is available.
| const stats = await this.ensureChromiumExists() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in a mirror - everything looks backwards and I still missed the obvious bugs.
| } | ||
|
|
||
| // Launch the browser | ||
| const stats = await this.ensureChromiumExists() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional? We're calling ensureChromiumExists() again on line 139 after already getting the stats on line 124. This creates duplicate work and could slow down browser launch.
| for (const dep of CHROME_DEPENDENCIES) { | ||
| try { | ||
| // Try to find the library using ldconfig | ||
| const { stdout } = await execAsync(`ldconfig -p | grep ${dep}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security concern: Using shell interpolation with grep ${dep} could be vulnerable if dependency names contain special characters. Consider using a safer approach:
| // Return the WebSocket endpoint | ||
| return "ws://localhost:3000" | ||
| } catch (error) { | ||
| console.error("Failed to start Docker browser:", error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Docker browser functions return null on error without logging. This makes debugging difficult. Could we add error logging here?
| } | ||
|
|
||
| // Wait for container to be ready | ||
| await new Promise((resolve) => setTimeout(resolve, 3000)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coded timeout of 3000ms might not be sufficient for slower systems. Could this be made configurable through settings?
| * Gets Docker browser configuration from settings | ||
| */ | ||
| export function getDockerBrowserConfig(context: vscode.ExtensionContext): DockerBrowserConfig { | ||
| const config = vscode.workspace.getConfiguration("roo-code") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: Should this be "roo-cline" instead of "roo-code" to match the other configuration keys?
| ### Browser Tool Support | ||
|
|
||
| The configuration ensures the browser tool works seamlessly in Codespaces by: | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation mentions "automatic" installation but doesn't clarify that sudo access is required. Could we add a note about this requirement to set proper expectations?
Summary
This PR addresses Issue #7990 by implementing automatic Chrome dependency installation and adding optional Docker-based browser isolation for GitHub Codespaces environments.
Problem
As reported by @LousyBook94, the browser tool was failing in GitHub Codespaces with missing Chrome/Puppeteer dependencies:
Solution
This PR implements a comprehensive solution that addresses all concerns raised:
1. Automatic Dependency Installation ✅
2. Optional Docker Container Support ✅
roo-cline.browserDocker.enabled: Enable/disable Docker browserroo-cline.browserDocker.image: Specify Docker image (default: browserless/chrome:latest)roo-cline.browserDocker.autoStart: Auto-start container when needed3. Enhanced Codespaces Support ✅
4. Intelligent Fallback Mechanism
The implementation uses a smart fallback chain:
Changes
New Files
src/services/browser/browserEnvironment.ts- Environment detection and dependency managementsrc/services/browser/__tests__/browserEnvironment.spec.ts- Comprehensive tests.devcontainer/devcontainer.json- Codespaces configuration.devcontainer/post-create.sh- Setup script for dependencies.devcontainer/post-start.sh- Verification script.devcontainer/README.md- Documentation and troubleshootingModified Files
src/services/browser/BrowserSession.ts- Integrated automatic dependency handlingsrc/services/browser/UrlContentFetcher.ts- Added environment detectionsrc/package.json- Added Docker browser settingssrc/package.nls.json- Added localization stringssrc/services/browser/__tests__/BrowserSession.spec.ts- Updated testsTesting
How to Test
In GitHub Codespaces:
With Docker (optional):
"roo-cline.browserDocker.enabled": trueVerify automatic dependency installation:
Documentation
Comprehensive documentation added in
.devcontainer/README.mdincluding:Fixes #7990
cc @LousyBook94 - This implementation addresses all your concerns about automatic dependency installation and the optional Docker containerization approach you suggested. The Docker option is now available in settings as requested!
Important
Adds automatic Chrome dependency installation and Docker browser support for GitHub Codespaces, with new settings and comprehensive testing.
BrowserSession.tsandUrlContentFetcher.tsfor Codespaces and containers.BrowserSession.tsif dependencies can't be installed.package.jsonfor Docker browser configuration.browserEnvironment.tsdetects Codespaces, container environments, and missing dependencies.startDockerBrowserandstopDockerBrowsermanage Docker container lifecycle.browserEnvironment.ts.browserEnvironment.spec.tsfor environment detection and Docker functions.BrowserSession.spec.tsfor new browser session logic.This description was created by
for dbe8a5f. You can customize this summary. It will automatically update as commits are pushed.